Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed creation of ControllerCreateVolumeRequest. #11238

Merged
merged 2 commits into from
Oct 6, 2021

Conversation

apollo13
Copy link
Contributor

CapacityRange should only be set if RequiredBytes and/or
LimitBytes are set.

@apollo13
Copy link
Contributor Author

This was discovered in https://gitlab.com/rocketduck/csi-plugin-nfs/-/issues/5

@lgfa29
Copy link
Contributor

lgfa29 commented Oct 2, 2021

Thanks for the fix @apollo13. I'm not too familiar with the CSI spec, but it doesn't seem to dictate any specific behaviour in this scenario:

// This field is OPTIONAL. This allows the CO to specify the capacity
// requirement of the volume to be provisioned. If not specified, the
// Plugin MAY choose an implementation-defined capacity range. If
// specified it MUST always be honored, even when creating volumes
// from a source; which MAY force some backends to internally extend
// the volume after creating it.

This issue interprets this as something that is up to each container orchestrator (CO) to decide, which is not great. Kubernetes seems to fail if the volume size is zero, as reported in this issue ceph/ceph-csi#2472, so I would expect most plugins to expect this behaviour from other COs as well.

Your approach would push the implementation details to plugins (If not specified, the Plugin MAY choose an implementation-defined capacity range) if Min and Max are not defined. Would that be OK or should an error be thrown?

@lgfa29 lgfa29 self-requested a review October 2, 2021 03:15
@lgfa29 lgfa29 added this to the 1.2.0 milestone Oct 2, 2021
@apollo13
Copy link
Contributor Author

apollo13 commented Oct 2, 2021

@lgfa29 Thank you for your response. I am also not sure what the best approach spec-wise would be. That said creating a volume currently fails due to:

nomad/plugins/csi/plugin.go

Lines 487 to 491 in 2998c4b

if r.CapacityRange != nil {
if r.CapacityRange.LimitBytes == 0 && r.CapacityRange.RequiredBytes == 0 {
return errors.New(
"one of LimitBytes or RequiredBytes must be set if CapacityRange is set")
}

So either my patch, relaxing the above check or more validation logic around volume create (ie already throw an error there alread) is needed.

Your approach would push the implementation details to plugins (If not specified, the Plugin MAY choose an implementation-defined capacity range) if Min and Max are not defined. Would that be OK or should an error be thrown?

It would be OK for me to push this to the plugins, then they can perform their own validation as needed.

@apollo13
Copy link
Contributor Author

apollo13 commented Oct 2, 2021

Thinking more about it I think the current code in nomad:

nomad/client/structs/csi.go

Lines 233 to 236 in f639f71

CapacityRange: &csi.CapacityRange{
RequiredBytes: req.CapacityMin,
LimitBytes: req.CapacityMax,
},

is indeed wrong. The spec clearly says that CapacityRange is optional, but if it is set at least one of the fields has to have a value. It also says that "A value of 0 is equal to an unspecified field value." for the fields in CapacityRange. So no matter what the CO or plugin later on does, the nomad clients should never submit a CapacityRange where both fields are zero because that would be a spec violation. As such I think my patch is the correct way and required.

`CapacityRange` should only be set if `RequiredBytes` and/or
`LimitBytes` are set.
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec clearly says that CapacityRange is optional, but if it is set at least one of the fields has to have a value. It also says that "A value of 0 is equal to an unspecified field value." for the fields in CapacityRange. So no matter what the CO or plugin later on does, the nomad clients should never submit a CapacityRange where both fields are zero because that would be a spec violation. As such I think my patch is the correct way and required.

Ah good catch, I missed that part from the spec, and that would actually be the behaviour I would expect.

I pushed a commit to document this requirement as a comment in the code and also to fix a small type in the CHANGELOG entry.

Thanks for catching and fixing this!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants